Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rename pallet-message-lane into pallet-bridge-messages #834

Merged
merged 3 commits into from
Mar 22, 2021
Merged

Rename pallet-message-lane into pallet-bridge-messages #834

merged 3 commits into from
Mar 22, 2021

Conversation

svyatonik
Copy link
Contributor

(this seems to be common convention after paritytech/substrate#8128)

Another (not super-important) thing is that our pallets have a bit different naming convention. We're going to include pallet-finality-verifier, pallet-bridge-call-dispatch and pallet-message-lane into P+K/W+R runtimes. Do we probably need to add that bridge- thing to all pallets? Or remove it from call dispatch pallet? This seems a bit sloppy now.

If we're going to add bridge- thing to all pallets names, then it worth doing it in this PR for ML pallet.

@tomusdrw
Copy link
Contributor

What about other pallets though (re: runtime prefix)? Let's maybe unify logging everywhere not just for ML.

Regarding naming I don't know if adding bridge- prefix actually solves the issue. Before the deployment I'd rather re-think the entire naming and maybe add a custom prefix as well:

  1. pallet-bridge-grandpa
  2. pallet-bridge-messages
  3. pallet-bridge-dispatch

would be my preference, but happy to discuss that in a dedicated issue.

@HCastano
Copy link
Contributor

What about other pallets though (re: runtime prefix)? Let's maybe unify logging everywhere not just for ML.

I agree here, we might as well change the logging targets in the other pallets. I think something like runtime::bridge::<target> would be ideal.

Regarding naming I don't know if adding bridge- prefix actually solves the issue. Before the deployment I'd rather re-think the entire naming and maybe add a custom prefix as well:

pallet-bridge-grandpa
pallet-bridge-messages
pallet-bridge-dispatch

would be my preference, but happy to discuss that in a dedicated issue.

Something like this seems reasonable as well, but yeah this is for a different issue/PR.

@svyatonik
Copy link
Contributor Author

svyatonik commented Mar 19, 2021 •
edited
Loading

It makes sense to me to rename pallet in this PR, because suffix in logs (at least now) depends on the pallet name - it is "runtime::<pallet-name-without-pallet-prefix>". So rename-PR will touch at least exactly the same code lines that this PR is touching. It also makes sense to me to rename other pallets in other PR - as I've already said logging suffix and pallet name better to be changed in single PR, so I do not want to rename half of repository in this PR. This PR is only about ML pallet.

So if we're ok with pallet-bridge-(grandpa|messages|dispatch), then I'm going to rename ML to pallet-bridge-messages and the logging target would be "runtime::bridge-messages".

@svyatonik svyatonik changed the title Use runtime:: prefix for message-lane pallet traces Rename pallet-message-lane into pallet-bridge-messages Mar 19, 2021
HCastano
HCastano approved these changes Mar 19, 2021
View reviewed changes
modules/messages/src/lib.rs
storage_keys::message_key::<TestRuntime, DefaultInstance>(&*b"test", 42).0,
hex!("87f1ffe31b52878f09495ca7482df1a48a395e6242c6813b196ca31ed0547ea79446af0e09063bd4a7874aef8a997cec746573742a00000000000000").to_vec(),
storage_key,
hex!("dd16c784ebd3390a9bc0357c7511ed018a395e6242c6813b196ca31ed0547ea79446af0e09063bd4a7874aef8a997cec746573742a00000000000000").to_vec(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik the storage key is computed as twox(pallet-name ++ storage-key-name) => then pallet name has been changed

@HCastano HCastano merged commit 5eb919e into master Mar 22, 2021
@HCastano HCastano deleted the fix-message-lane-pallet-tracingf branch March 22, 2021 16:28
@HCastano HCastano mentioned this pull request Mar 22, 2021
Merged
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Mar 27, 2024
@svyatonik
Rename pallet-message-lane into pallet-bridge-messages (paritytech#834)
96159d2 
* use runtime:: prefix for message-lane pallet traces

* renamed message-lane (module and primitives) folder into messages

* replace "message lane" with "messages" where appropriate
serban300 pushed a commit to serban300/parity-bridges-common that referenced this pull request Apr 8, 2024
@svyatonik
Rename pallet-message-lane into pallet-bridge-messages (paritytech#834)
a6c8bd9 
* use runtime:: prefix for message-lane pallet traces

* renamed message-lane (module and primitives) folder into messages

* replace "message lane" with "messages" where appropriate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomusdrw tomusdrw tomusdrw approved these changes

@HCastano HCastano HCastano approved these changes

Assignees
No one assigned
Labels
P-Message Delivery P-Runtime
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@svyatonik @tomusdrw @HCastano